[6/7] Add native batch integration coverage - #1633
Conversation
|
JDBC integration tests triggered ( |
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium
Test-only PR (one benign debug log + WireMock replay fixtures + a new native-batch integration test). Cleanup logic correctly mirrors the sibling THRIFT+REPLAY skip pattern. One medium concern: the SEA request-body assertion uses equalToJson(..., true, true) and omits parameter values, so it does not actually verify the ordering it claims to.
Scope: reviewed as a stacked increment (8b0bfbb..head); the parent's changes are reviewed on the parent PR.
|
Integration test approval reset. New commits were pushed to this PR. Label(s) A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.) Latest commit: 07968b8 |
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — test-only PR (fake-service replay coverage for native PreparedStatement batching) plus one benign debug log line. The cleanUp() condition correctly mirrors the sibling close-unless-THRIFT+REPLAY convention, and the SEA verify() uses ignoreArrayOrder=false to properly assert ordered parameter_sets. One low-severity robustness note on the Thrift routing workaround in setUp.
Scope: reviewed as a stacked increment (8b0bfbb..head); the parent's changes are reviewed on the parent PR.
|
JDBC integration tests triggered ( |
sreekanth-db
changed the title
Snapshot prepared parameter sets at addBatch time and route eligible batches through an injectable native executor while preserving the legacy production path. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Accept the immutable parameter map produced by the shared batch model instead of requiring a concrete HashMap. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Route eligible prepared batches through compute-aware Thrift batchParameters, preserve legacy compatibility fallback, and map repeat-compressed backend results to ordered JDBC update counts. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Surface post-success count-read errors as RESULT_SET_ERROR and clear committed batches before propagating so retries cannot duplicate inserted rows. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Submit ordered parameter_sets through the existing SEA execution lifecycle so opt-in prepared batches share the same routing, fallback, and JDBC result handling as Thrift. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Record real SEA and Thrift responses to guard native parameter batching end to end. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Validate exact parameter values and array order so replay coverage detects reordered native batches. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
07968b8 to
2d0f4da
Compare
|
Integration test approval reset. New commits were pushed to this PR. Label(s) A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.) Latest commit: 2d0f4da |
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — test-only coverage plus a safe debug-log line; 1 low note. The recorded SEA/Thrift stubs and the ordered parameter_sets verify align with the stated intent, and cleanup mirrors the existing ExecutionIntegrationTests idiom. Only concern is the unguarded/unrestored system-property rewrite in setUp (F1).
Scope: reviewed as a stacked increment (07f3135..head); the parent's changes are reviewed on the parent PR.
| String targetUri = System.getProperty("thrift_server.targetURI"); | ||
| String routeProperty = targetUri + FAKE_SERVICE_URI_PROP_SUFFIX; | ||
| System.setProperty( | ||
| routeProperty, System.getProperty(routeProperty).replace("localhost", "127.0.0.1")); |
There was a problem hiding this comment.
🔵 Low — The THRIFT branch reads System.getProperty(routeProperty) and calls .replace(...) on it without a null guard, then overwrites the global property without ever restoring it. If the route property is unset for any reason (e.g. the test is run in a configuration where FakeServiceExtension.setFakeServiceProperties didn't populate it), this throws a bare NullPointerException with no context rather than a clear failure. The mutation is also never reverted; it happens once per @BeforeEach and persists on the shared JVM system properties until the extension's onAfterAll clears the key. The localhost→127.0.0.1 swap is idempotent so it's functionally harmless in practice, but the unguarded deref plus the absence of an explanatory comment (why the rewrite is needed for THRIFT+REPLAY) makes this fragile. Consider guarding the lookup and adding a one-line comment explaining the WireMock/IPv6-vs-IPv4 rationale.
1 participant
🥞 Stacked PR
Use this link to review incremental changes.
Description
parameter_setsrequest and both transports return ordered JDBC update counts.Testing
FAKE_SERVICE_TYPE=SQL_EXEC FAKE_SERVICE_TEST_MODE=REPLAY mvn test -pl jdbc-core -Dtest=NativePreparedStatementBatchIntegrationTests -Ddependency-check.skip=trueFAKE_SERVICE_TYPE=THRIFT_SERVER FAKE_SERVICE_TEST_MODE=REPLAY mvn test -pl jdbc-core -Dtest=NativePreparedStatementBatchIntegrationTests -Ddependency-check.skip=truemvn test -pl jdbc-core -Dtest=PreparedStatementBatchExecutorTest,DatabricksSdkClientTest,DatabricksThriftServiceClientTest -Ddependency-check.skip=true(119 tests)isaac review --uncommitted(0 findings)Additional Notes to the Reviewer
Only real recorded native-success responses are included. Compatibility fallback remains covered by unit tests because the available backends support native batching and do not emit the legacy fallback signal.
NO_CHANGELOG=true